-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ready-for-review: Un-hardcode in governance parameters #1688
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1688 +/- ##
===========================================
+ Coverage 63.42% 63.47% +0.04%
===========================================
Files 117 117
Lines 6940 6973 +33
===========================================
+ Hits 4402 4426 +24
- Misses 2283 2292 +9
Partials 255 255 |
Alternative to merging this for gaia 7000 would be to just change the voting period to a larger number |
x/gov/keeper.go
Outdated
} | ||
|
||
func (keeper Keeper) setDepositProcedure(ctx sdk.Context, depositProcedure DepositProcedure) { | ||
keeper.ps.Set(ctx, "gov/depositprocedure", &depositProcedure) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these strings should be well defined constants at top of file.
Also might be nicer as gov/procedure/deposit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
Threshold: sdk.NewRat(1, 2), | ||
Veto: sdk.NewRat(1, 3), | ||
GovernancePenalty: sdk.NewRat(1, 100), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any other point to splitting up the governance parameters into these Procedure
structs besides organizational? If not I'd almost prefer to see all the parameters in one struct - seems a bit confusing to add another layer of abstraction where it's not a requirement of codes functionality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Voting params, DepositParams, and TallyingParams are used at different times. No need to Unmarshal the voting params when I'm just worried about tallying. Also, in future, there will be different types of "tallying params" and stuff for different types of proposals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting so in the future tallying params should actually be an interface
as opposed to a struct
to allow for different parameters - then functions such as CheckVeto
could be functions on TallyingProceedure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 seems fine to leave as is for now, thanks for the insight
Can you update your dep, and re-run make get_vendor_deps and see if that makes your gopkg.lock match? |
I did that. It still gave me this Gopkg.lock |
@sunnya97 you've been slashed! - for including the checkbox applied appropriate labels.. when really no labels have been applied! |
Ohhh, I thought that meant to the title of the PR lol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PENDING.md & dep upgrade, plus file conflicts, otherwise LGTM.
CHANGELOG.md
Outdated
@@ -13,6 +13,8 @@ BREAKING CHANGES | |||
|
|||
FEATURES | |||
* [baseapp] NewBaseApp now takes option functions as parameters | |||
* [x/gov] Governance parameters are now stored in globalparams store |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to PENDING.md
@@ -2,78 +2,59 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update dep version & re-run make get_vendor_deps
.
5ddc9d0
to
874440c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
moved governance parameters to globalparams store and can be set in genesis
docs/
)CHANGELOG.md
cmd/gaia
andexamples/
For Admin Use: